PatternFly-React preview: https://2176-pr-patternfly-react-patternfly.surge.sh

Thanks for posting this! I'm cc'ing @seanforyou23 on my comment to get his input on some things. There are some aspects of this update that are still open questions that I'd like other opinions on, and then there are a few minor things that I noticed.

Open questions:

1. Default value for headingLevel prop - In a slack discussion, @seanforyou23 had made this comment which is a great point:

   I think it's problematic to default Title to h1 and we've already seen this cause issues across various apps where all the headings are now h1 and it doesn't accurately depict their information architecture.

   We have a larger team meeting tomorrow, which would be a great time to get feedback from more devs on this.

Miscellaneous issues:

1. headingLevel prop on Accordion and AccordionToggle - Currently, this prop is listed in the documentation for both components, but I only see it working when I specify it on the AccordionToggle component. Instead, we should have this as a prop on the Accordion component only. Since all headings in this structure should have the same level, putting the prop here makes it easier for the consumer to specify it.

2. consistency of documentation - We should try to be as consistent as possible with how we handle similar props across different components. The following is how a similar prop is listed for the Title component. The main column here that I question is the Type. I think we'd want to use what's listed for Type in the Title component, but would like @seanforyou23 to comment on this.

   Name	Type	Required	Default	Description
   headingLevel	'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'		'h1'	the heading level to use

@jgiardino @seanforyou23 unless we default to h3 it would be considered a breaking change

I was wondering if that would be the case. I was trying to think of a situation where changing the heading level from h3 to h1 would break something? The styles wouldn't be affected unless they had created custom styles based on the html element. And if that's the only case, I think it could be argued whether or not we would support that. But I could also see the case for a general rule of changing element type being considered a breaking change, just to simplify things.

Anyway, I'm totally fine with defaulting to the default we have now (<h3>) for this PR and changing the default later. 😄

@jenny-s51 just a heads up, we're going to have merge conflicts between this PR and my TypeScript conversion of the Accordion (#2121) so depending on whose PR gets merged first, either you or I will need to port your changes over to the TypeScript version.

@jenny-s51 just a heads up, we're going to have merge conflicts between this PR and my TypeScript conversion of the Accordion (#2121) so depending on whose PR gets merged first, either you or I will need to port your changes over to the TypeScript version.

sounds good, thank you for this info

I'm recapping here some of what we discussed last week...

@jschuler had raised the question about whether heading levels are required for this pattern, and if just the <dl> is semantically sufficient.

Here are a few examples of accordions out there:
https://www.w3.org/TR/wai-aria-practices-1.1/#accordion
https://inclusive-components.design/collapsible-sections/
https://www.scottohara.me/blog/2017/10/25/accordion-release.html
https://a11yproject.com/patterns/

The first 3 use headings, without a definition list. The last link includes an example, "Simple Accordion", that uses a definition list and not headings.

Our pattern uses both, but instead, we should use one or the other. Our core code supports not including a heading, but it doesn't support not using the <dl> elements.

My strongest opinion about this is that we should support the example provided by the WAI-ARIA authoring practices, and will continue to want to have a variation that uses headings.

I will capture an issue in core to create an example that allows for just headings and no definition list.

For this PR, I think we can move forward with allowing the consumer to specify the heading list, using the structure that we currently have. But I also defer to the main contributors to this repo on what they would recommend, if we expect other changes to be needed based on updates in core.

Last week, @seanforyou23 had shared some thoughts about what the default heading level should be in the case where the consumer doesn't specify one. I'm not sure that's something that can/should be addressed in this PR. Here are some comments on that:

• I like the suggestion of using a lower heading level as a general approach to default heading levels. There's less potential for conflict with the heading outline in the case where the consumer doesn't define it if we use <h5> vs something like <h3>.
• As mentioned above, we could wait to make changes to the default value as part of a breaking change
• If we do decide to provide the definition list variation, then could headingLevel be used to both use the variation with headings and define the heading level (i.e. if not defined, we use the definition list variation)?

Are we still planning to have one headingLevel prop in <Accordion> and have the children consume it via Context instead of requiring the consumer to pass it to each child separately? https://reactjs.org/docs/context.html

BTW, here is the issue that I created in core: patternfly/patternfly#1913

Are we still planning to have one headingLevel prop in and have the children consume it via Context instead of requiring the consumer to pass it to each child separately?

That would be my preference.

It's strange that your branch has so many commits from other PRs.. did you merge master into your branch? It's probably better to do a git rebase instead of a git merge when you're updating your PR with the latest master, to keep the commit history straightforward.

Closing this. New PR #2273 has been opened to resolve the issue.